perf: parallel generate, speed up 10% #1001
Conversation
WalkthroughThe changes introduce new dependencies, refactor chunk file generation logic, enhance file processing, and add functionality for concurrent operations. Configuration options for production mode and chunk hashing are now available. Changes
Poem
Note Pull Request Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://coderabbit.ai TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (11)
- crates/mako/Cargo.toml (1 hunks)
- crates/mako/src/chunk_pot.rs (7 hunks)
- crates/mako/src/chunk_pot/util.rs (1 hunks)
- crates/mako/src/generate_chunks.rs (2 hunks)
- crates/mako/src/thread_pool.rs (1 hunks)
- e2e/fixtures/generate.chunks-hash-replace/expect.js (1 hunks)
- e2e/fixtures/generate.chunks-hash-replace/mako.config.json (1 hunks)
- e2e/fixtures/generate.chunks-hash-replace/src/index.css (1 hunks)
- e2e/fixtures/generate.chunks-hash-replace/src/index.ts (1 hunks)
- e2e/fixtures/generate.chunks-hash-replace/src/lazy.css (1 hunks)
- e2e/fixtures/generate.chunks-hash-replace/src/lazy.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- e2e/fixtures/generate.chunks-hash-replace/src/index.css
Additional Context Used
Additional comments not posted (14)
e2e/fixtures/generate.chunks-hash-replace/src/lazy.ts (1)
1-2: The changes inlazy.tsare straightforward and correct. It properly imports a CSS file and exports a string value.e2e/fixtures/generate.chunks-hash-replace/mako.config.json (1)
1-4: Themako.config.jsonfile is correctly formatted and the configuration settings align with the PR's objectives of optimizing the build process.e2e/fixtures/generate.chunks-hash-replace/src/lazy.css (1)
1-5: The CSS changes inlazy.cssare syntactically correct and follow standard CSS practices.e2e/fixtures/generate.chunks-hash-replace/src/index.ts (1)
1-3: The changes inindex.tsare correct, including the dynamic import of thelazymodule, which aligns with modern JavaScript practices for lazy-loading.e2e/fixtures/generate.chunks-hash-replace/expect.js (1)
1-18: The assertions inexpect.jsare correctly implemented to test the chunk hash replacement functionality, aligning with the PR's objectives of ensuring the integrity of the new parallel generation mechanism.crates/mako/src/thread_pool.rs (1)
29-39: The addition of thejoinfunction inthread_pool.rsis correctly implemented to facilitate parallel execution of operations using the Rayon thread pool, adhering to Rust's concurrency best practices.crates/mako/Cargo.toml (1)
21-21: The addition of thenanoiddependency with version0.4.0inCargo.tomlis correctly specified and aligns with the PR's requirements for generating temporary placeholders for chunk hashes.crates/mako/src/chunk_pot.rs (2)
28-28: Making thestylesheetfield public in theChunkPotstruct aligns with the PR's objectives of enhancing chunk processing.
186-200: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [133-197]
The adjustments to return types and making the
CssModulesstruct andget_css_chunk_filenamefunction public are correctly implemented, facilitating the new functionality introduced in the PR.crates/mako/src/chunk_pot/util.rs (1)
307-313: The introduction ofCHUNK_FILE_NAME_HASH_LENGTHto standardize hash lengths across the application is a good practice. However, it's crucial to ensure that the chosen length of 8 characters strikes a balance between uniqueness and brevity to minimize the risk of hash collisions. Consider analyzing the potential for collisions with the expected volume of files and adjusting the length if necessary.crates/mako/src/generate_chunks.rs (4)
11-11: The use ofnanoidfor generating placeholders and the introduction ofChunksHashPlaceholderandChunksHashReplacerare well-considered choices for managing the complexity of parallel chunk generation. Ensure to conduct performance testing to assess the impact ofnanoidgeneration and analyze edge cases, such as the unlikely but possible generation of identical placeholders for different chunks.Also applies to: 55-56
64-74: The parallel generation of entry and normal chunk files usingthread_pool::joinis a significant improvement in the build process. Ensure thorough testing is conducted to identify any potential data races or concurrency issues, especially considering the shared state that might be accessed by these parallel tasks.
150-156: The handling of CSS filenames and placeholders within the chunk generation process is well-integrated. Ensure that the naming and replacement logic for CSS files is reviewed for consistency and efficiency, particularly considering the unique requirements and conventions for CSS files compared to JS chunks.Also applies to: 158-161
285-285: Thehash_file_namefunction is crucial for generating hashed filenames. Ensure that edge cases, such as files without extensions or with multiple dots in their names, are adequately tested to guarantee the function's robustness in all scenarios.
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/mako/src/generate_chunks.rs (2 hunks)
Additional Context Used
Additional comments not posted (5)
crates/mako/src/generate_chunks.rs (5)
11-11: The addition ofnanoid::nanoidaligns with the PR's objective to use temporary placeholders for chunk hashes. This is a lightweight and efficient way to generate unique IDs, suitable for the intended use case.
55-56: The introduction ofChunksHashPlaceholderandChunksHashReplacermaps is a good design choice. It clearly separates the concerns of holding placeholders for chunk hashes and their eventual replacement values, contributing to code readability and maintainability.
72-114: The parallel generation of entry and normal chunk files usingthread_pool::joinis a significant improvement. It leverages concurrency to enhance performance, directly addressing the PR's objective. However, ensure that the thread pool is appropriately sized to avoid potential overhead from too many threads competing for resources.
81-114: The logic for replacing chunk hash placeholders with actual hashes is well-implemented. It's crucial, however, to ensure that the replacement operation does not introduce any unintended modifications to the chunk files. Consider adding more detailed tests to cover edge cases, such as placeholders appearing in unexpected parts of the chunk content.
255-298: Thereplace_chunks_placeholderfunction is crucial for the new chunk generation logic. It replaces placeholders in chunk files with actual hashes. The error handling here is specific and informative, which is good practice. However, consider the performance implications of iterating over the content for each placeholder. For large files or a high number of placeholders, this could become a bottleneck. Investigating more efficient string replacement techniques or ensuring that the number of placeholders is kept minimal could mitigate potential performance issues.
xusd320
changed the title
xusd320
force-pushed
the
perf/parallel-generate
branch
from
d8cd5bc to
d6a0632
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (11)
- crates/mako/Cargo.toml (1 hunks)
- crates/mako/src/chunk_pot.rs (7 hunks)
- crates/mako/src/chunk_pot/util.rs (1 hunks)
- crates/mako/src/generate_chunks.rs (2 hunks)
- crates/mako/src/thread_pool.rs (1 hunks)
- e2e/fixtures/generate.chunks-hash-replace/expect.js (1 hunks)
- e2e/fixtures/generate.chunks-hash-replace/mako.config.json (1 hunks)
- e2e/fixtures/generate.chunks-hash-replace/src/index.css (1 hunks)
- e2e/fixtures/generate.chunks-hash-replace/src/index.ts (1 hunks)
- e2e/fixtures/generate.chunks-hash-replace/src/lazy.css (1 hunks)
- e2e/fixtures/generate.chunks-hash-replace/src/lazy.ts (1 hunks)
Files skipped from review as they are similar to previous changes (10)
- crates/mako/Cargo.toml
- crates/mako/src/chunk_pot.rs
- crates/mako/src/chunk_pot/util.rs
- crates/mako/src/thread_pool.rs
- e2e/fixtures/generate.chunks-hash-replace/expect.js
- e2e/fixtures/generate.chunks-hash-replace/mako.config.json
- e2e/fixtures/generate.chunks-hash-replace/src/index.css
- e2e/fixtures/generate.chunks-hash-replace/src/index.ts
- e2e/fixtures/generate.chunks-hash-replace/src/lazy.css
- e2e/fixtures/generate.chunks-hash-replace/src/lazy.ts
Additional Context Used
Additional comments not posted (5)
crates/mako/src/generate_chunks.rs (5)
11-11: The addition ofnanoidas a dependency for generating unique placeholders is a good choice due to its efficiency and minimal collision risk. Ensure that the version0.4.0is compatible with other dependencies and the project's overall ecosystem.
55-56: IntroducingChunksHashPlaceholderandChunksHashReplacerasHashMaptypes is a clear and efficient way to manage the mapping between chunk IDs and their respective placeholders or actual hash values. This approach supports the parallel generation and replacement logic effectively.
72-115: The parallel generation of entry and normal chunk files usingthread_pool::joinis a significant improvement. It aligns with the PR's objective to enhance performance through concurrency. However, ensure that the thread pool is adequately sized to prevent potential bottlenecks or excessive resource usage.
81-115: The logic for replacing placeholders in entry chunk files with actual hash values is well-implemented. It's crucial to ensure that this replacement process is thoroughly tested, especially in edge cases where placeholders might not be found or replaced correctly.
256-299: Thereplace_chunks_placeholderfunction is crucial for the new parallel generation logic, ensuring that placeholders in chunk files are replaced with actual hash values. It's important to handle errors gracefully and provide clear messages for debugging. Consider adding more detailed logging or metrics around this process to help identify and troubleshoot issues in production environments.
xusd320
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/mako/src/generate_chunks.rs (2 hunks)
Additional Context Used
Additional comments not posted (5)
crates/mako/src/generate_chunks.rs (5)
11-11: Importingnanoidaligns with the PR objectives for efficient chunk hashing. Ensure that thenanoidversion (v0.4.0) is compatible with the project's other dependencies to avoid conflicts.
55-56: The introduction ofChunksHashPlaceholderandChunksHashReplacertypes is a good practice for managing chunk hash placeholders and their replacements. This enhances code readability and maintainability.
72-115: The parallel generation of entry and normal chunk files usingthread_pool::joinis a significant improvement. It aligns with the PR's goal of speeding up the build process. However, ensure that the error handling within the parallel execution paths is robust and that any potential panics are gracefully handled.
81-115: The logic for replacing chunk hash placeholders in entry chunk files is well-implemented. However, consider adding more detailed logging or metrics around the placeholder replacement process to aid in debugging and performance monitoring.
256-299: Thereplace_chunks_placeholderfunction is crucial for the dynamic replacement of chunk hash placeholders. It's well-implemented, but consider adding error logging for each specific failure case to aid in troubleshooting. Additionally, ensure that the performance impact of iterating over chunk files and performing replacements is minimal, especially for large builds.
xusd320
changed the title
xusd320
changed the title
Close #976
改动点：
验证性能提升：
yuyanAssets 提升 10%
当前分支:
master:
Summary by CodeRabbit
New Features
Refactor
Documentation
mako.config.jsonto include new configuration options.Style